Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WIP] rpc: recycle request/response memory across gRPC calls #137806

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

[WIP] rpc: recycle request/response memory across gRPC calls #137806

wants to merge 2 commits into from
+365 −2

Conversation

nvanbenschoten
Copy link
Member

This commit creates a better grpc.PreparedMsg and uses it in both directions of the BatchStream RPC.

grpc.PreparedMsg was introduced in this commit (see grpc/grpc-go#2432) as a way to provide more control over message encoding, but it doesn't actually allow for memory reuse. Notably, each use of PreparedMsg.Encode still allocates all memory afresh. As far as I can tell, the only thing it does is allow for parallel serialization/compression.

In this commit, we exploit the API with a few sprinkles of unsafe magic and our own implementation to allow for reuse of the memory buffers used for serializing and compressing requests and responses.

name                                            old time/op    new time/op    delta
Sysbench/KV/1node_remote/oltp_point_select-10     33.8µs ±10%    31.2µs ± 4%   -7.94%  (p=0.002 n=10+9)
Sysbench/KV/1node_remote/oltp_read_only-10         839µs ± 2%     814µs ± 3%   -2.96%  (p=0.000 n=9+10)
Sysbench/KV/1node_remote/oltp_read_write-10       1.76ms ± 1%    1.73ms ± 3%   -1.56%  (p=0.015 n=10+10)
Sysbench/SQL/1node_remote/oltp_read_only-10       3.66ms ± 7%    3.53ms ± 7%     ~     (p=0.077 n=9+9)
Sysbench/SQL/1node_remote/oltp_read_write-10      5.80ms ± 3%    5.75ms ± 1%     ~     (p=0.173 n=10+8)
Sysbench/SQL/1node_remote/oltp_point_select-10     186µs ±13%     178µs ± 4%     ~     (p=0.105 n=10+10)

name                                            old alloc/op   new alloc/op   delta
Sysbench/KV/1node_remote/oltp_read_only-10         880kB ± 0%     664kB ± 0%  -24.63%  (p=0.000 n=10+10)
Sysbench/KV/1node_remote/oltp_read_write-10       1.24MB ± 1%    0.99MB ± 2%  -20.06%  (p=0.000 n=10+10)
Sysbench/SQL/1node_remote/oltp_read_only-10       1.37MB ± 0%    1.17MB ± 0%  -14.70%  (p=0.000 n=9+10)
Sysbench/KV/1node_remote/oltp_point_select-10     8.77kB ± 3%    7.71kB ± 0%  -12.08%  (p=0.000 n=9+8)
Sysbench/SQL/1node_remote/oltp_read_write-10      1.90MB ± 0%    1.68MB ± 0%  -11.87%  (p=0.000 n=10+10)
Sysbench/SQL/1node_remote/oltp_point_select-10    33.8kB ± 2%    31.8kB ± 3%   -5.83%  (p=0.000 n=9+10)

name                                            old allocs/op  new allocs/op  delta
Sysbench/KV/1node_remote/oltp_point_select-10       65.0 ± 0%      55.0 ± 0%  -15.38%  (p=0.002 n=8+10)
Sysbench/KV/1node_remote/oltp_read_write-10        4.39k ± 0%     4.08k ± 0%   -7.02%  (p=0.000 n=9+10)
Sysbench/KV/1node_remote/oltp_read_only-10         1.94k ± 0%     1.81k ± 0%   -6.68%  (p=0.000 n=10+10)
Sysbench/SQL/1node_remote/oltp_read_write-10       8.66k ± 0%     8.33k ± 0%   -3.82%  (p=0.000 n=9+9)
Sysbench/SQL/1node_remote/oltp_point_select-10       257 ± 0%       248 ± 1%   -3.66%  (p=0.000 n=9+10)
Sysbench/SQL/1node_remote/oltp_read_only-10        4.64k ± 0%     4.50k ± 0%   -3.18%  (p=0.000 n=9+10)

Don't merge unless we're ready to abuse grpc on a whole new level. It is probably better just to fork at this point. Or we could try to upgrade to a newer version of gRPC that contains grpc/grpc-go#6619.

Epic: None
Release note: None

@nvanbenschoten
[DNM] rpc: enable compression even for loopback transport
358359b 
Don't merge, but helpful for benchmarking.
@nvanbenschoten
[WIP] rpc: recycle request/response memory across RPC calls
bb87c14 
This commit creates a better `grpc.PreparedMsg` (one that actually recycles
memory) and uses it in both directions of the BatchStream RPC.

Epic: None
Release note: None
@nvanbenschoten nvanbenschoten added the o-perf-efficiency Related to performance efficiency label Dec 20, 2024
@nvanbenschoten nvanbenschoten requested a review from tbg December 20, 2024 01:00
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tbg tbg Awaiting requested review from tbg

Assignees
No one assigned
Labels
o-perf-efficiency Related to performance efficiency
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nvanbenschoten @cockroach-teamcity